refactor: update document version handling and add SuperDoc ID [SD-428]#814
refactor: update document version handling and add SuperDoc ID [SD-428]#814harbournick merged 15 commits intomainfrom
Conversation
harbournick
left a comment
There was a problem hiding this comment.
Looks good and I like the ideas. Just a few comments for clarity
|
@edoversb @harbournick I think we need a more deep analysis here before merging this |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors document identifier handling by introducing a new document GUID system alongside existing version management. The changes transition from a single document ID approach to separate handling of permanent GUIDs (for modified documents) and temporary hashes (for unmodified documents), improving telemetry tracking and document lifecycle management.
- Renamed
updateDocumentVersiontosetDocumentVersionand improved error handling for custom properties - Added document GUID and identifier methods to SuperConverter with proper precedence (Microsoft GUID > custom GUID > hash)
- Enhanced telemetry tracking with separate document GUID and identifier fields
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/common/Telemetry.js | Updated telemetry fields from single superdocId to separate documentGuid and documentIdentifier with hash generation removal |
| packages/super-editor/src/core/super-converter/exporter-docx-defs.js | Removed unused SETTINGS_CUSTOM_XML export |
| packages/super-editor/src/core/super-converter/SuperConverter.test.js | Added comprehensive test coverage for new GUID/identifier resolution and promotion functionality |
| packages/super-editor/src/core/super-converter/SuperConverter.js | Major refactor adding document GUID management, identifier resolution, and generic custom property handling |
| packages/super-editor/src/core/Editor.js | Added document modification tracking, GUID promotion, and telemetry data methods with backward compatibility |
Comments suppressed due to low confidence (1)
packages/super-editor/src/core/super-converter/SuperConverter.js:1
- The method signature has changed to add a new
documentIdentifierparameter between existing parameters, which is a breaking change. Consider adding the new parameter at the end to maintain backward compatibility, or provide method overloading to handle both old and new signatures.
import xmljs from 'xml-js';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
harbournick
left a comment
There was a problem hiding this comment.
Just a few comments. Overall it looks good to me. However main points:
- Need to check if this works in collaboration
- Need to test what happens if converter.docx = [] (ie: we are not in docx mode)
- Please add unit tests to all new functions
- We need to create a few test files to test the various cases (missing GUID etc). How can we add this to automated testing (either within this repo, or in the separate tests repo)
β¦n methods - Renamed `updateDocumentVersion` to `setDocumentVersion` for clarity. - Introduced methods to get and set SuperDoc ID and version in `SuperConverter`. - Improved error handling and structure for custom properties in docx. - Added unit tests for new functionality in `SuperConverter`.
- Introduced a two-tier identification system for documents, utilizing a permanent GUID for modified documents and a temporary hash for unmodified ones. - Updated methods to retrieve and promote document identifiers, ensuring backward compatibility with deprecated methods. - Removed unused settings custom XML structure and streamlined document version handling. - Enhanced telemetry to track document identifiers and updated related tests for comprehensive coverage.
- Eliminated the getDocumentId method to streamline the Editor class, as it is no longer needed following recent changes to document identifier handling. - This refactor contributes to cleaner code and improved maintainability.
β¦Converter - Updated the Editor class to simplify document modification tracking and promote to GUID when necessary. - Refactored methods for retrieving document identifiers, ensuring clarity and consistency in handling GUIDs and hashes. - Deprecated outdated methods for backward compatibility while enhancing telemetry data retrieval. - Adjusted related tests to accommodate asynchronous changes in document identifier generation.
- Updated the SuperConverter class to safely access the 'Properties' element in custom XML, ensuring that the code handles cases where the element may not exist. - This change improves the robustness of the document conversion process by preventing potential runtime errors.
- Added radix parameter to parseInt for clarity when converting PIDs. - Replaced isFinite check with Number.isInteger to ensure PIDs are strictly integers, enhancing the robustness of the PID generation logic.
- Enhanced the getDocumentGuid method to parse XML instead of using regex for better accuracy and reliability. - Implemented null checks to prevent errors when accessing XML elements, improving the robustness of the document conversion process.
- Included a comment in the SuperConverter class to clarify that the GUID is stored in custom properties during export to prevent unnecessary XML modifications if the document is not saved. This enhances code readability and understanding of the export process.
8b1519e to
68841d3
Compare
|
π This PR is included in version 0.23.0-next.15 π The release is available on:
Your semantic-release bot π¦π |
|
π This PR is included in version 0.23.0 π The release is available on: Your semantic-release bot π¦π |
updateDocumentVersiontosetDocumentVersionfor clarity.SuperConverter.SuperConverter.Notes: